fix(h2): escape SQL identifiers and literals in metadata/DDL paths (#1914) - #2177
Conversation
openai0229
left a comment
openai0229
left a comment
There was a problem hiding this comment.
Thanks for hardening these second-order SQL injection paths. The escaping behavior added here is needed, but H2SqlEscapes creates a second dialect-escaping abstraction. The Community SPI already defines ISQLIdentifierProcessor as the owner of both identifier quoting (quoteIdentifier) and SQL string-literal escaping (escapeString). H2 currently inherits DefaultSQLIdentifierProcessor through DefaultMetaService.
Please implement an H2-specific identifier processor (or strengthen the appropriate shared processor), return it from H2Meta#getSQLIdentifierProcessor(), and reuse that processor from H2Meta, H2DBManager, and H2SqlBuilder instead of routing these call sites through H2SqlEscapes.
Simply switching to the current DefaultSQLIdentifierProcessor is not sufficient: its quoteIdentifier only wraps invalid names and does not double embedded double quotes, while its escapeString preserves adjacent quote pairs instead of encoding every quote in a raw value. The processor implementation therefore needs focused coverage for embedded identifier quotes, already quoted identifiers, consecutive literal quotes, and case-sensitive H2 metadata names. Identifier quoting and string-literal escaping should remain separate processor methods.
This keeps one reusable dialect contract and prevents future H2 SQL-building call sites from bypassing the hardening.
|
Would you be willing to take this opportunity to improve this class of architectural issue more broadly, rather than limiting the change to the seven H2 call sites in this diff? It looks likely that identifier quoting and SQL string-literal escaping have drifted away from This is a broader improvement request, so please let us know whether you would be willing to take it on as part of this work, or as focused follow-up work if the scope is too large for this PR. |
|
Agreed — thank you for the direction. I've reworked this branch onto the SPI contract:
I will roll the same pattern out to the sibling PRs (#2172-#2176 and #2194-#2206): each plugin gets its escaping consolidated into its own |
|
Rollout complete: all 19 sibling PRs (#2172-#2176, #2194-#2206) now share the same shape as this branch — escaping consolidated into each plugin's ISQLIdentifierProcessor implementation (strengthened existing processors where present), getSQLIdentifierProcessor() overridden on the MetaData side, non-escapable validation in per-plugin SqlGuards. One extra finding from our own adversarial pass during the rollout, worth flagging: making quoteIdentifier unconditionally quote broke SPI consumers (GenericSqlCompletionEngine relies on conditional-quote semantics + null passthrough). The final contract is therefore two-track: quoteIdentifier stays conditional (null->null, valid plain identifiers unquoted, fold-aware for PG) and a plugin-level quoteIdentifierAlways serves DDL-generation sites. Backtick processors override removeIdentifierQuote/isQuoteIdentifier accordingly. All module test suites green (incl. mysql 631 tests). |
|
Thanks for completing the rollout. I reviewed the current heads of all 19 PRs. Moving the escaping implementations into plugin processors is the right direction, but the rollout is not merge-ready yet. The main common blocker is that identifier quote/remove round-trip is still broken across the 18 relational branches. For example, a raw identifier such as There are also contract and dialect regressions in the current heads:
Please add a shared processor conformance test class and run every relational processor through it. At minimum it should cover:
Please also add plugin-level SQL/DDL semantic tests for legal default expressions and structural breakout inputs. Where an embedded or test-container database is available, execute the generated DDL rather than only asserting string fragments. SQLite should prove that a type such as The shared SPI contract and conformance suite should be fixed first, then the sibling branches should be rebased onto it. Otherwise the same contract bug has to be corrected independently in every PR. |
|
To make the next step concrete, please pause the per-dialect rollout for now. PR #2234 is already introducing quoteIdentifierAlways into ISQLIdentifierProcessor, so the shared contract should be settled there (or in one equivalent foundation PR) before updating 19 branches independently. The foundation should include the shared conformance suite described above, especially the quote/remove round-trip and conditional-versus-always-quote semantics. Once that lands, please rebase H2 and MySQL first as reference implementations and add executable DDL/default-expression tests. After those two are accepted, the remaining relational dialect branches can be rebased onto the same contract. Generic and MongoDB should remain separate follow-ups: Generic cannot enforce one identifier/expression grammar across arbitrary SQL dialects, while MongoDB needs JavaScript/command-structure-aware validation rather than SQL identifier rules. This gives us one stable contract to review instead of correcting the same behavior independently across 19 branches. |
openai0229
left a comment
openai0229
left a comment
There was a problem hiding this comment.
Rebased onto #2234 and verified the H2 implementation end to end. Identifier quote/remove round trips, conditional versus always-quote behavior, metadata and builder call sites, structural type/default rejection, and legal H2 default semantics are now covered. The real H2 metadata export executes successfully in a second database. Local clean reactor result: 30 H2 tests, 0 failures or errors.
…tterMind#1914) Override quoteIdentifier/quoteQualifiedIdentifier/buildTableName/buildColumns and reimplement buildCreateTable/buildAlterTable/buildUpdate/buildTemplate/ buildCreateDatabase in H2SqlBuilder so every identifier is double-quote escaped and every comment literal is single-quote escaped. Validate metadata TYPE_NAME against an allow-list and neutralize hostile COLUMN_DEF defaults in generated DDL. Apply the export SCRIPT NODATA sentinel before substituting the escaped schema name so schema names containing NODATA are not corrupted. Document the raw-name contract on H2SqlEscapes and add attack-string tests for the builder, manager and metadata paths.
…tterMind#1914) Route column.getColumnType() in buildCreateTable and generateAlterColumnSql (ADD/MODIFY) through H2SqlEscapes.requireSafeTypeName so a hostile type string cannot break out of generated DDL. Adds attack-string rejection tests.
… review (OtterMind#1914) - new H2IdentifierProcessor (SPI ISQLIdentifierProcessor): quoteIdentifier with double-quote doubling, escapeString with single-quote doubling - H2Meta overrides getSQLIdentifierProcessor(); metadata call sites use it - builders/managers use H2IdentifierProcessor.INSTANCE - non-escapable validation moved to H2SqlGuards (type names, column defaults) - H2SqlEscapes removed; tests migrated (27 green)
…h, conditional for SPI, always for DDL) (OtterMind#1914) The pilot branch predated the dual-track contract; its always-quote processor had the same null->"" garbage and completion regression the other modules were fixed for.
2 participants
Related issue
Part of #1914. Depends on the shared identifier contract merged in #2234.
Summary
ISQLIdentifierProcessor#quoteIdentifierAlwayscontract.H2IdentifierProcessorwith H2-aware conditional quoting, unconditional DDL quoting, embedded double-quote round trips, and raw string-literal escaping.DATA_TYPE, precision, and scale instead of treating display width as a type argument. This avoids invalid exports such asBIGINT(64)andTIMESTAMP(26).Affected surfaces
Verification
mvn -B -f chat2db-community-server/pom.xml -pl chat2db-community-plugins/chat2db-community-h2 -am -Dmaven.test.skip=false -DskipTests=false clean testcompleted withBUILD SUCCESS; H2 tests: 30 run, 0 failures, 0 errors, 0 skipped.git diff --checkalso passed.H2Meta, executes it in a second H2 database, inserts default values, and verifies sequence, timestamp, and string defaults.Risk and compatibility
Reviewer map
H2IdentifierProcessor,H2SqlGuards, andH2Meta; then review the escaped call sites inH2SqlBuilderandH2DBManager.Contributor declaration
AI assistance: The implementation, adversarial test cases, compatibility review, and PR description were produced with AI coding assistance and maintainer review.